Skip to content

fix(agent): execute decoded JSON after escaped-nonascii resample budget - #4627

Merged
Yeachan-Heo merged 1 commit into
devfrom
fix/ask-tool-ascii
Aug 18, 2026
Merged

fix(agent): execute decoded JSON after escaped-nonascii resample budget#4627
Yeachan-Heo merged 1 commit into
devfrom
fix/ask-tool-ascii

Conversation

@Yeachan-Heo

@Yeachan-Heo Yeachan-Heo commented Aug 17, 2026

Copy link
Copy Markdown
Owner

What

The escaped-non-ASCII argument guard keeps its fail-closed terminal rejection and its unconditional two-resample budget for every tool and every field. After the budget is spent, a tool that enumerated its user-facing display fields (displaySafeEscapedArgFields; ask exempts only questions.question and questions.options.label) executes when every non-ASCII character lives inside those fields and is benign typographic punctuation (curated set: U+2014 EM DASH only).

Why

Deep-interview ask prompts whose only non-ASCII was an em-dash were rejected after two resamples, so the user never saw the question. Both maintainer reviews (4957008249, 4958187304) shaped this reduction:

  1. Terminal guard retained — mutating tools and arbitrary text never execute unverified \uXXXX payloads.
  2. Budget runs unconditionally — the exemption moved to terminal execution, after MAX_ESCAPED_NONASCII_RESAMPLES; the gate skip is gone (major 1 resolved).
  3. Field-scoped, not whole-objectask opts in only questions.question and questions.options.label; ids, deep-interview metadata (intent contracts/reviews/references), and workflow-gate metadata keep the fail-closed rejection, and any escaped non-ASCII outside the enumerated fields rejects (major 2 resolved). Non-ASCII object keys are never exempted.
  4. No any — the helper takes AgentTool<TSchema> (nit resolved).
  5. Rebased onto live dev (ceb31349); the packages/ai/CHANGELOG.md conflict is resolved keeping both entries (merge precondition resolved).

Behavior after this change

  • escaped em-dash in an ask question: 1 original + 2 resamples, then executes (question reaches the user) — the motivating fix.
  • the same payload on a non-enumerating tool: budget, then terminal \uXXXX rejection.
  • em-dash in deepInterview/ids/any non-display field of ask: budget, then terminal rejection (field-scoped).
  • nibble-adjacent U+2013/U+2015/U+2027/U+2043, currency ₩, math ≈, full-width !, ideographic space, Hangul, emoji, lone surrogates: rejected everywhere, including on ask.
  • mixed batch: turn resamples; after budget the benign ask call executes while the non-safe call rejects terminally, per-call.

Testing

29 tests in the escaped-nonascii suite (4 original red-team + after-budget execution + field-scope negative + mixed-batch), plus a 9-case external adversarial harness (astral emoji, non-ASCII object keys, deep nesting, mixed batches, unknown tool fail-closed, nibble-adjacent dashes, lone surrogate, schema-ordering) — all pass.

env -u OPENAI_BASE_URL -u ANTHROPIC_BASE_URL -u ANTHROPIC_AUTH_TOKEN bun test \
  <7-file escaped-nonascii matrix>                          # 126 pass / 0 fail
env -u OPENAI_BASE_URL bun test packages/agent/test/        # 791 pass / 0 fail
bun --cwd=packages/{ai,agent,coding-agent} run check         # clean (biome + tsc)
bun scripts/verify-gjc-state-writers.ts --fail --root .      # 0 violations
bun scripts/changelog-history-guard.ts                       # clean
git diff --check                                             # clean
bun --cwd=packages/coding-agent run build && dist/gjc --version && --smoke-test  # ok

Binary SHA-256 (same builder/toolchain): base ceb31349e6e53364… lineage; head a00ff6f4a30aab82…053.

Attribution: single commit authored as Yeachan-Heo <yeachan-heo@gajae.dev> on live dev (ceb31349). Every earlier head (3353a67, a2d1a5c8, ae592d88, 03a33380, c0d1bf9d), verdict, and CI run is void.

GJC verdict

Fresh exact head a00ff6f431dd6333f8aeed5f7e7b01a13a49cf29, base ceb31349c2d024c75825c819cb530d5060f49380. No approving review on this head yet.

gajae.pr-review-verdict.v1 merge-approved sha256:3342ae3f566af11dd51da79d35e6bba3ebb8aa5da4d5119b2d1b1df36d5a2313 reviewer:human reviewer-id:probepark evidence:exact-head-a00ff6f4-exemption-post-budget-single-codepoint-allowlist-closes-nibble-argument-68-pass-0-fail

Approved by @probepark at exact head a00ff6f4 (authenticated review 4963160294, write authority, independent non-author) with the merge-approved verdict line quoted above.


  • Target branch is dev
  • bun check passes (per-package run check = biome + tsc, all three touched packages clean)
  • Tested locally
  • CHANGELOG updated (if user-facing)
  • Verdict above matches the exact PR head, not an earlier commit

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@Yeachan-Heo

Copy link
Copy Markdown
Owner Author

Rebased onto current dev and re-verified — review requested on the new exact head a2d1a5c8c8.

  • Original commit cherry-picked verbatim (author Yeachan-Heo <yeachan-heo@gajae.dev>, message unchanged). Only delta change vs the old head is one packages/ai/CHANGELOG.md context line from fix(omlx): keep role thinking effort on discovered models #4631; no code conflicts.
  • Exact-head verification: 127 pass / 0 fail (7-file targeted matrix), 782 pass / 0 fail (full packages/agent), 106 pass / 0 fail (ai escaped/healing + openai-completions-compat), per-package run check clean, build + --version + --smoke-test ok.
  • Binary diff SHA-256 (same builder/toolchain): base 6696988b6d058260d4…3770b; head a2d1a5c8c8ed6c7758…b2abb.
  • Adversarial review complete (2-pass Architect CLEAR/APPROVE + Critic OKAY): decoded-JSON execution keeps incompleteArguments/validateToolArguments/beforeToolCall ordering; bidi/invisible format codepoints (U+202E/U+200B/U+FEFF/U+00AD/U+0085, C1 block) stay flagged; letters/marks/numbers and completed surrogate pairs still flag; retry budget 2 with per-turn reset; all 9 provider adapters use the shared raw-payload scanner.
  • Verdict remains needs-human (owner-authored; self-approval prohibited). An independent admin/maintain/write approval on a2d1a5c8c8 flips it to merge-approved naming you and unblocks the contract checks + merge.
gajae.pr-review-verdict.v1 needs-human sha256:ffeeb33030a10fb292f7990459f4f4d3bb682eb0fe3845d3e5a4d9c22baeb168 reviewer:human reviewer-id:Yeachan-Heo evidence:exact-head-a2d1a5c8c8-rebased-127+782+106-tests-checks-build-smoke-binary-diff

@Yeachan-Heo

Copy link
Copy Markdown
Owner Author

Digest correction on the exact head a2d1a5c8c8 — the earlier comment's ffeeb330… figure was computed against the fetched-at-the-time base before the push finalized the immutable event base 6696988b6d. The canonical exact-head diff digest (locally recomputed and confirmed by the contract check) is:

gajae.pr-review-verdict.v1 needs-human sha256:dc13f89c1b11a584d7464250e1ee52a679ff2302cd47c9de0a41d6fcf7dc6964 reviewer:human reviewer-id:Yeachan-Heo evidence:exact-head-a2d1a5c8c8-rebased-127+782+106-tests-checks-build-smoke-binary-diff

The PR body now carries this digest (single verdict line). Verification matrix unchanged: 127 + 782 + 106 tests pass, per-package checks clean, build/smoke ok, binary base 058260d4… → head ed6c7758…. Still needs-human pending an independent write-capable approval on a2d1a5c8c8 (@probepark @snowykr requested).

@Yeachan-Heo

Copy link
Copy Markdown
Owner Author

Second reconstruction onto live dev — new exact head ae592d88b151672d91a9bf205ab3e7e8f3da81e9 (base 7265a61c8ed4). All prior heads/verdicts/approvals/CI are stale and void.

  • fix(agent): bound managed staging by reclaiming superseded deltas #4610/fix(session): stop resume listing paying one read syscall per 4 KiB #4616 landed overlapping agent-loop.ts regions, so the original commit was cherry-picked onto current dev with author Yeachan-Heo and author date 2026-08-17 20:11:55 +0900 preserved; CHANGELOG siblings semantically merged; git diff --check clean.
  • Exact-head verification: 127 pass/0 fail (7-file targeted matrix), 783 pass/0 fail (full packages/agent), 106 pass/0 fail (ai escaped/healing/compat), per-package run check clean, verify-gjc-state-writers --fail → 0 violations, changelog-history-guard → no released sections removed, build + --version + --smoke-test ok.
  • Binary SHA-256: base 7265a61c8ed44a791aa0…e8cf; head ae592d88b16072bc0c…090e1.
  • Replacement CI is running on this head. Verdict is needs-human with reviewer-id:pending; a fresh independent non-author approval on ae592d88b1 (requested: @probepark @snowykr) is the only path to merge-approved + merge.
gajae.pr-review-verdict.v1 needs-human sha256:7f04cac562c23d98708ad82ec84455c6e36062964cce995dbb8f3dee75448c83 reviewer:human reviewer-id:pending evidence:exact-head-ae592d88b1-on-7265a61c-127+783+106-tests-checks-writers-changelog-diffcheck-build-smoke-binary

@probepark probepark left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent maintainer review — merge blocked. This trades a false positive for a real safety regression.

major — the terminal guard is removed for every tool, not just ask

packages/agent/src/agent-loop.ts:2361-2364,3473-3491 drops terminal escaped-argument validation globally. After two unmanaged resamples the decoded arguments are executed as-is, so a single mistyped nibble can silently alter a non-ASCII path, source text, regex, or shell argument and then run a mutating tool. The em-dash false positive does not justify executing still-unverified arguments.

major — the encoder exemption is far too broad

packages/ai/src/utils/json-parse.ts:28-37,203-206 treats all of \p{P}, \p{S}, and \p{Z} as harmless. Currency and math symbols, full-width punctuation, and separators are semantically significant and nibble-sensitive in arbitrary tool arguments.

Fix the em-dash case narrowly — evidence-backed, non-mutating fields only — and keep the terminal path fail-closed.

conflict with #4632

#4632 touches the same hunks with the opposite design (keep the guard, add transient steering so the model re-emits literal UTF-8). The two are mutually exclusive. I recommend landing #4632's direction and reducing this PR to just the em-dash false-positive fix.

Yeachan-Heo pushed a commit that referenced this pull request Aug 18, 2026
…nly tools

Restore the fail-closed terminal escapedNonAsciiArguments rejection for
every tool, then carve out exactly the motivating false positive: a tool
that declares displaySafeEscapedArgs (user-facing question text that
names no path, command, or identifier — currently only ask) executes
without resampling when its decoded arguments contain nothing but benign
typographic punctuation (curated set: U+2014 EM DASH). The json-parse
scanner stays strict and still flags every non-ASCII escape; mutating
tools and every other escaped character keep the two-resample budget and
terminal rejection. Composes with the retained-guard + steering
direction of #4632, which owns transient recovery steering.

Red-team regressions cover: em-dash executes only on display-safe tools;
the same payload on a mutating tool never executes; the nibble-adjacent
en-dash (U+2013) and currency/math/full-width/separator/letter/emoji
escapes stay rejected even on display-safe tools.

Lore-id: 0f3c2a91
Constraint: mutating tools must never execute unverified \\uXXXX payloads
Constraint: scanner stays evidence-based; the exemption is decided on decoded args at execution time
Rejected: broad \p{P}\p{S}\p{Z} exemption | semantically significant nibble-sensitive symbols re-execute
Rejected: execute decoded args after budget globally | review 4957008249 major blocker
Confidence: high
Scope-risk: narrow
Reversibility: trivial
Tested: 28 escaped-nonascii agent tests, 787 agent suite, 125 targeted matrix, 100 ai targeted
Not-tested: none material
Closes: #4627 review blockers
@Yeachan-Heo

Copy link
Copy Markdown
Owner Author

Review resolution for 4957008249 — new exact head 03a333806bc3dda57a0b37af337c6564e5718968 on live dev 27afb732b3d. All three blockers addressed by reduction, not rebuttal.

Blocker 1 — terminal guard removed globally → RESTORED. The fail-closed escapedNonAsciiArguments rejection in executeToolCalls is back for every tool. No unverified decoded arguments execute anywhere outside the single carve-out below; mutating tools (write/edit/bash) reject terminally after the budget, exactly as before this PR existed.

Blocker 2 — broad \p{P}\p{S}\p{Z} exemption → REMOVED. The scanner is byte-identical to dev and still flags every non-ASCII escape (pinned by a new test: \u2014, \u201c, \u00a0, \u20a9, \uff01 all still flag). The exemption now lives at execution time against the decoded arguments, gated on both (a) the tool explicitly declaring displaySafeEscapedArgs — user-facing question text that names no path, command, or identifier; currently only ask — and (b) a curated codepoint set of exactly U+2014 EM DASH, the evidence-backed motivating false positive. Currency ₩, math ≈, full-width !, separators  , letters 안, marks, numbers, and emoji surrogates stay rejected even on the display-safe tool; red-team tests pin each of these plus the nibble-adjacent U+2013 EN DASH.

Blocker 3 — conflict with #4632 → COMPOSES. #4632 retains the guard and adds transient recovery steering (its lane owns that); this PR adds only the bounded display-safe carve-out on disjoint logic — a DISPLAY_SAFE_ESCAPED_CODEPOINTS membership check against decoded args at the resample gate and the terminal guard. No hunk overlap in semantics: steering still fires for every non-benign escape, and the carve-out only ever skips resampling for payloads that would execute anyway. Reduced per your recommendation to "just the em-dash false-positive fix."

Verification on 03a33380: 125/0 targeted matrix, 787/0 full packages/agent, 100/0 ai targeted, per-package run check clean (ai/agent/coding-agent), verify-gjc-state-writers --fail 0 violations, changelog-history-guard clean, git diff --check clean, build + --version + --smoke-test ok. Binary SHA-256: base 27afb732b3 81064392…11aa → head 03a33380 451abf0c…1d30. Single commit, author Yeachan-Heo <yeachan-heo@gajae.dev> preserved.

Fresh exact-head re-review requested: @probepark @snowykr.

gajae.pr-review-verdict.v1 needs-human sha256:ef034f5af2ca2621d773dd01b59135ebc7f4290ce343e8b05bde541d8c12cf9f reviewer:human reviewer-id:pending evidence:exact-head-03a33380-on-27afb732-125+787+100-tests-checks-writers-changelog-diffcheck-build-smoke-binary


[repo owner's gaebal-gajae (clawdbot) 🦞]

@Yeachan-Heo
Yeachan-Heo requested a review from probepark August 18, 2026 05:21
Yeachan-Heo pushed a commit that referenced this pull request Aug 18, 2026
…nly tools

Restore the fail-closed terminal escapedNonAsciiArguments rejection for
every tool, then carve out exactly the motivating false positive: a tool
that declares displaySafeEscapedArgs (user-facing question text that
names no path, command, or identifier — currently only ask) executes
without resampling when its decoded arguments contain nothing but benign
typographic punctuation (curated set: U+2014 EM DASH). The json-parse
scanner stays strict and still flags every non-ASCII escape; mutating
tools and every other escaped character keep the two-resample budget and
terminal rejection. Composes with the retained-guard + steering
direction of #4632, which owns transient recovery steering.

Red-team regressions cover: em-dash executes only on display-safe tools;
the same payload on a mutating tool never executes; the nibble-adjacent
en-dash (U+2013) and currency/math/full-width/separator/letter/emoji
escapes stay rejected even on display-safe tools.

Lore-id: 0f3c2a91
Constraint: mutating tools must never execute unverified \\uXXXX payloads
Constraint: scanner stays evidence-based; the exemption is decided on decoded args at execution time
Rejected: broad \p{P}\p{S}\p{Z} exemption | semantically significant nibble-sensitive symbols re-execute
Rejected: execute decoded args after budget globally | review 4957008249 major blocker
Confidence: high
Scope-risk: narrow
Reversibility: trivial
Tested: 28 escaped-nonascii agent tests, 787 agent suite, 125 targeted matrix, 100 ai targeted
Not-tested: none material
Closes: #4627 review blockers
@Yeachan-Heo

Copy link
Copy Markdown
Owner Author

Reconstructed onto live dev — exact head c0d1bf9d2b8b07c6d0b0b40b9a76ec38f4b2594c, base 416201eb5e. Product validation from 03a33380 completed green; the three-dot diff is byte-identical (zero file overlap with the 33 dev commits), so the committed digest ef034f5a… remains exact.

Verification re-run on the new head: 125/0 targeted escaped-nonascii matrix, 787/0 full packages/agent, per-package run check clean (ai/agent/coding-agent), verify-gjc-state-writers --fail 0 violations, changelog-history-guard clean, git diff --check clean, build + --version + --smoke-test ok. Binary SHA-256: base 416201eb5e e6e53364…2b59 → head c0d1bf9d 94360975…3def. Author Yeachan-Heo <yeachan-heo@gajae.dev> preserved; author/committer dates intact.

The review-resolution comment below (all three blockers of 4957008249) applies verbatim to this head — the change set is identical. Fresh exact-head re-review still requested: @probepark @snowykr. On a fresh non-author approval the verdict flips to merge-approved naming the reviewer, contract checks rerun, and the PR merges.

gajae.pr-review-verdict.v1 needs-human sha256:ef034f5af2ca2621d773dd01b59135ebc7f4290ce343e8b05bde541d8c12cf9f reviewer:human reviewer-id:pending evidence:exact-head-c0d1bf9d-on-416201eb-125+787-0-agent-suite-checks-writers-changelog-diffcheck-build-smoke-binary


[repo owner's gaebal-gajae (clawdbot) 🦞]

@probepark probepark left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent maintainer review at exact head c0d1bf9dmerge blocked. The design moved in the right direction since my last review, but the code does not do what the title says.

what improved

The global terminal-guard removal is gone. The retry counter is bounded (MAX_ESCAPED_NONASCII_RESAMPLES = 2), malformed decoded JSON is caught first via incompleteArguments (agent-loop.ts:3528-3547) and produces a defined terminal error, schema validation still runs before dispatch (3582-3593), and completed surrogate pairs are treated as a single escaped scalar (json-parse.ts:153-164) so astral characters are rejected rather than split or executed. The broad \p{P}/\p{S}/\p{Z} exemption I blocked on is gone. Public types are not breakingly reshaped — packages/ai/src/types.ts is documentation-only, packages/agent/src/types.ts adds one optional property.

major 1 — the exemption runs before the budget, not after it

packages/agent/src/agent-loop.ts:2418-2430: the new message.content.every(...) clause skips the resample gate entirely for eligible calls. Zero resamples, straight to decoded execution.

The PR is titled "execute decoded JSON after escaped-nonascii resample budget". This is not that. It also contradicts AgentTool.displaySafeEscapedArgs's own documentation at packages/agent/src/types.ts:704-710. The whole justification for tolerating hand-escaped arguments is that the model was given its two chances to emit literal UTF-8 first; deleting that step removes the pressure that makes the fallback rare and turns it into the default path.

Move the exemption to terminal execution, after MAX_ESCAPED_NONASCII_RESAMPLES is exhausted.

major 2 — Ask opts its entire argument object into a display-only contract

packages/coding-agent/src/tools/ask.ts:737-743. The contract claims the data is display-only. Ask arguments are not: they carry IDs, deep-interview metadata, intent contracts and reviews, references, and workflow-gate metadata (ask-contract.ts:64-143), and some of it is persisted at ask.ts:767-805.

Because agent-loop.ts:219-239 accepts U+2014 anywhere in any nested key or value, an escaped em dash appearing in durable metadata now bypasses the guard too. A nibble error there corrupts recorded consent evidence, not a rendered question.

Make the exemption field/path-aware and scope it to actual question and option display strings. The Ask coupling does belong in this PR — a concrete tool has to opt in for the mechanism to mean anything — but not at whole-object granularity.

nit

agent-loop.ts:219: AgentTool<any> violates the no-any contract. Use AgentTool or a narrow structural type.

coverage — the one positive test pins the wrong behavior

packages/agent/test/agent-loop-escaped-nonascii-toolcall.test.ts:952-977 fails against the prior loop, so it is doing work, but it asserts mock.calls length 2 — it explicitly pins the zero-resample shortcut. Once major 1 is fixed this test must change.

There is no test where two escaped responses consume the budget and a third decoded response executes, which is the entire claimed behavior. The mutating-tool, en-dash, and currency/math/full-width/separator/letter/emoji rejection tests all pass on the prior fail-closed implementation. The added packages/ai/test/json-parse.test.ts scanner test also passes without source changes, since the scanner is intentionally unchanged.

merge precondition

Head conflicts with devpackages/ai/CHANGELOG.md only, no code conflict. Rebase and recompute the digest; the verdict is bound to c0d1bf9d.

Reviewed by @probepark — method: detached worktree at c0d1bf9d, full read of the resample loop and terminal path in agent-loop.ts, surrogate-pair handling trace in json-parse.ts, exemption-surface audit against ask-contract.ts field inventory and the ask.ts persistence path, per-test would-this-fail-on-base analysis. Tests not executed.

gajae.pr-review-verdict.v1 merge-blocked sha256:ef034f5af2ca2621d773dd01b59135ebc7f4290ce343e8b05bde541d8c12cf9f reviewer:human reviewer-id:probepark evidence:exact-head-c0d1bf9d-exemption-applied-pre-budget-and-whole-object-ask-optin

Yeachan-Heo pushed a commit to lee98www/moss-gajae-code that referenced this pull request Aug 18, 2026
…ack continuation

Review of the reconstructed change found the steering installed only
after the fallbackManaged branch returns, so coding-agent sessions
(which run managed and own their retry through the session policy)
kept re-issuing the defective request blind. The deterministic Hangul
escaper cited in the PR body never saw the instruction.

The typed escaped_arguments_discarded outcome now reports whether the
discarded attempt still lacked a transient recovery instruction, and
the session's retry continuation attaches exactly one such instruction
through a new transientRecoveryMessage prompt option, consumed by the
loop as a one-shot synthetic recovery on the first assistant request of
the continuation. The instruction stays provider-only (never durable
history, never a later request) and the terminal per-call rejection
remains fail-closed; Yeachan-Heo#4627's decode-and-execute direction is still
rejected.

Lore-id: b7d2e4f1
Constraint: instruction must never enter durable history -- rides the existing synthetic recoveryMode path
Constraint: steering is one-shot per logical turn; never re-attach after a steered attempt was itself discarded
Rejected: steering every managed retry | a steered attempt that still escapes proves the instruction is not working; repeating it burns budget on identical requests
Rejected: decoding the escapes and executing | one mistyped nibble is unverifiable after parse
Confidence: high
Scope-risk: narrow
Reversibility: trivial
Tested: managed AgentSession regression (fails without the fix); all 25 agent escaped-guard tests; 790 agent package tests
Not-tested: none known
…nly tools

Restore the fail-closed terminal escapedNonAsciiArguments rejection for
every tool, then carve out exactly the motivating false positive: a tool
that declares displaySafeEscapedArgs (user-facing question text that
names no path, command, or identifier — currently only ask) executes
without resampling when its decoded arguments contain nothing but benign
typographic punctuation (curated set: U+2014 EM DASH). The json-parse
scanner stays strict and still flags every non-ASCII escape; mutating
tools and every other escaped character keep the two-resample budget and
terminal rejection. Composes with the retained-guard + steering
direction of #4632, which owns transient recovery steering.

Red-team regressions cover: em-dash executes only on display-safe tools;
the same payload on a mutating tool never executes; the nibble-adjacent
en-dash (U+2013) and currency/math/full-width/separator/letter/emoji
escapes stay rejected even on display-safe tools.

Lore-id: 0f3c2a91
Constraint: mutating tools must never execute unverified \\uXXXX payloads
Constraint: scanner stays evidence-based; the exemption is decided on decoded args at execution time
Rejected: broad \p{P}\p{S}\p{Z} exemption | semantically significant nibble-sensitive symbols re-execute
Rejected: execute decoded args after budget globally | review 4957008249 major blocker
Confidence: high
Scope-risk: narrow
Reversibility: trivial
Tested: 28 escaped-nonascii agent tests, 787 agent suite, 125 targeted matrix, 100 ai targeted
Not-tested: none material
Closes: #4627 review blockers
Yeachan-Heo pushed a commit to lee98www/moss-gajae-code that referenced this pull request Aug 18, 2026
…ack continuation

Review of the reconstructed change found the steering installed only
after the fallbackManaged branch returns, so coding-agent sessions
(which run managed and own their retry through the session policy)
kept re-issuing the defective request blind. The deterministic Hangul
escaper cited in the PR body never saw the instruction.

The typed escaped_arguments_discarded outcome now reports whether the
discarded attempt still lacked a transient recovery instruction, and
the session's retry continuation attaches exactly one such instruction
through a new transientRecoveryMessage prompt option, consumed by the
loop as a one-shot synthetic recovery on the first assistant request of
the continuation. The instruction stays provider-only (never durable
history, never a later request) and the terminal per-call rejection
remains fail-closed; Yeachan-Heo#4627's decode-and-execute direction is still
rejected.

Lore-id: b7d2e4f1
Constraint: instruction must never enter durable history -- rides the existing synthetic recoveryMode path
Constraint: steering is one-shot per logical turn; never re-attach after a steered attempt was itself discarded
Rejected: steering every managed retry | a steered attempt that still escapes proves the instruction is not working; repeating it burns budget on identical requests
Rejected: decoding the escapes and executing | one mistyped nibble is unverifiable after parse
Confidence: high
Scope-risk: narrow
Reversibility: trivial
Tested: managed AgentSession regression (fails without the fix); all 25 agent escaped-guard tests; 790 agent package tests
Not-tested: none known
@Yeachan-Heo

Copy link
Copy Markdown
Owner Author

Review 4958187304 resolution — new exact head a00ff6f431dd6333f8aeed5f7e7b01a13a49cf29 on live dev ceb31349. Every finding addressed:

Major 1 (exemption ran before the budget) → FIXED. The message.content.every(...) gate-skip clause is deleted; the resample gate is byte-identical to base again (hasEscapedNonAsciiToolCall only). The exemption now lives solely in the terminal path in executeToolCalls, i.e. it applies only after MAX_ESCAPED_NONASCII_RESAMPLES is exhausted. The PR-title behavior is now literal: 1 original + 2 resamples, then decoded execution for the bounded case. New test executes the benign em-dash ask case AFTER the resample budget pins mock.calls == 4 — the exact consume-budget-then-execute sequence you asked for.

Major 2 (whole-object ask opt-in) → FIXED. displaySafeEscapedArgs: boolean is replaced by displaySafeEscapedArgFields: readonly string[]; ask enumerates exactly ["questions.question", "questions.options.label"]. The walker now enforces the inverse too: any non-ASCII outside the enumerated fields rejects (not just "enumerated fields are benign"), non-ASCII object keys reject unconditionally, and deepInterview/ids/workflowGate metadata — including everything persisted at ask.ts:767-805 — keeps the fail-closed rejection. New test never exempts escaped non-ASCII outside the declared display fields pins an em-dash inside deepInterview.dimension rejecting while the same em-dash in question would pass.

Nit (AgentTool<any>) → FIXED. Signature is now AgentTool<TSchema> | undefined via a structural DisplaySafeEscapedTool type; no any.

Coverage. The zero-resample pin was replaced by the after-budget test. Mixed-batch behavior is pinned: the turn resamples (not every call display-safe), then per-call the benign ask executes while the non-safe call rejects terminally. External 9-case adversarial harness re-run against the new semantics: astral emoji, non-ASCII object keys, deep-nested Hangul, unknown tool (fail-closed before tool resolution), nibble-adjacent U+2015/U+2027/U+2043, lone surrogate, and schema-ordering (carve-out does not bypass validateToolArguments) — all pass.

Merge precondition → RESOLVED. Rebased onto ceb31349 (past the packages/ai/CHANGELOG.md conflict, both entries kept; also past #4633/#4655 which touch adjacent agent-loop.ts regions — no semantic overlap, full suite re-run green). GitHub now reports MERGEABLE.

Verification on a00ff6f4: 126/0 escaped-nonascii matrix, 791/0 full packages/agent, per-package run check clean, writers gate 0 violations, changelog guard clean, git diff --check clean, build + smoke ok. Binary: base-lineage e6e53364… → head a30aab82…053.

gajae.pr-review-verdict.v1 needs-human sha256:3342ae3f566af11dd51da79d35e6bba3ebb8aa5da4d5119b2d1b1df36d5a2313 reviewer:human reviewer-id:pending evidence:exact-head-a00ff6f4-on-ceb31349-126+791-tests-checks-writers-changelog-diffcheck-build-smoke-binary-redteam9

Fresh exact-head re-review requested: @probepark @snowykr.


[repo owner's gaebal-gajae (clawdbot) 🦞]

@Yeachan-Heo
Yeachan-Heo requested a review from probepark August 18, 2026 15:11

@probepark probepark left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review at exact head a00ff6f4approved. Both majors are fixed, and fixed properly rather than argued away. This is a materially different PR from the one I blocked.

major 1 — exemption is now genuinely post-budget

The message.content.every(...) clause is gone from the resample gate. agent-loop.ts:2442-2448 now reads exactly as before the PR: any escaped tool call consumes a resample, unconditionally, up to MAX_ESCAPED_NONASCII_RESAMPLES. The model always gets its chances to re-emit literal UTF-8.

The exemption moved to agent-loop.ts:3565, inside executeToolCalls — a path only reached once the budget is spent. That is what the PR title claimed all along and now what the code does.

major 2 — the opt-in is field-scoped, and tighter than I asked for

ask.ts declares displaySafeEscapedArgFields = ["questions.question", "questions.options.label"]. Ids, deep-interview metadata, intent contracts and reviews, references, and workflow-gate metadata all stay fail-closed, including on the persisted path. That was the whole of my objection.

But the part I want to credit properly is DISPLAY_SAFE_ESCAPED_CODEPOINTS = new Set([0x2014]).

My original argument against tolerating primitives was that a mistyped nibble silently becomes a different valid character. A one-element allowlist closes that by construction: mistype any nibble of \u2014 and you get a codepoint that is not U+2014, which is not in the set, so the call is rejected. The failure mode I was defending against cannot occur. Combined with the path check — every non-ASCII character must be in a declared display field and be that one codepoint — the residual surface is a single typographic dash in text the user is about to read.

That is a real answer to the objection, not a narrower version of the same bet.

relationship to #4632

I approved #4632 (transient steering, guard fully fail-closed) earlier today, and these two touch the same hunks, so whichever lands second needs a rebase.

They are complementary rather than competing now: #4632 removes most escaped payloads before they ever reach terminal, and this PR handles the residue where the only offender is an em dash in display text — so a user's turn is not killed by a dash the model refused to stop escaping. I would land #4632 first, since it fixes the cause, then rebase this on top; the exemption gets exercised less often but is still the right terminal behavior.

I am withdrawing my earlier recommendation to close this in favor of #4632. At the head I reviewed then, that was the right call. At this head it is not.

coverage

The tests are the strongest part of the change:

  • executes the benign em-dash ask case AFTER the resample budget on a display-safe tool
  • never exempts escaped non-ASCII outside the declared display fields
  • never executes the same em-dash payload when the tool is not display-safe
  • rejects a nibble-adjacent symbol escape even on a display-safe tool
  • rejects currency, math, full-width, separator, letter, and emoji escapes on a display-safe tool
  • the json-parse scanner still flags em dash and everything else — the exemption lives in the loop, not the scanner

The fourth one is the test I would have demanded, and it is there. The last one correctly pins that the scanner stays evidence-based, so the exemption cannot leak into a lower layer.

Ran locally: bun test packages/agent/test/agent-loop-escaped-nonascii-toolcall.test.ts packages/ai/test/json-parse.test.ts68 pass, 0 fail, 215 assertions.

sweep

No added any, ReturnType<>, inline dynamic imports, console.*, or mock.module(). Both changelog entries under ## [Unreleased].

Reviewed by @probepark — method: detached worktree at a00ff6f4, verification that the resample gate no longer short-circuits, confirmation the exemption sits in executeToolCalls past the budget, read of the codepoint set and path matcher to test the mistyped-nibble argument against the new design, field-inventory cross-check against ask-contract.ts, focused test run.

gajae.pr-review-verdict.v1 merge-approved sha256:3342ae3f566af11dd51da79d35e6bba3ebb8aa5da4d5119b2d1b1df36d5a2313 reviewer:human reviewer-id:probepark evidence:exact-head-a00ff6f4-exemption-post-budget-single-codepoint-allowlist-closes-nibble-argument-68-pass-0-fail

@Yeachan-Heo
Yeachan-Heo merged commit 648d13d into dev Aug 18, 2026
153 of 184 checks passed
@Yeachan-Heo
Yeachan-Heo deleted the fix/ask-tool-ascii branch August 18, 2026 20:00
@Yeachan-Heo

Copy link
Copy Markdown
Owner Author

TERMINAL: merged to dev — squash commit 648d13d72d0749c068eb9278e04f2a6ba0b07b78 (2026-08-18T20:00:35Z).

  • Exact head merged: a00ff6f431dd6333f8aeed5f7e7b01a13a49cf29 (single commit, author Yeachan-Heo <yeachan-heo@gajae.dev> preserved through the squash attribution).
  • Approval chain: @probepark APPROVED at exact head (review 4963160294, write authority, independent non-author); contract checks green on the merge-approved verdict digest 3342ae3f…; Dev CI aggregate green (all affected-path validation, state gates, native build, cli-smoke, virtual integration, ts-build — zero failures at merge time).
  • Canonical origin/dev verified at 648d13d72d with the PR head as an ancestor.
  • Post-merge dogfood on fresh dev: bun run build ok → binary 51ab3910878ad724838e6054aa8a0f482c938bb890a0420c5b262ce67251d51b; --versiongjc/0.14.0; --smoke-test → ok; focused suites on merged dev: 68 pass / 0 fail (escaped-nonascii + json-parse).

Terminal verdict line for the record:

gajae.pr-review-verdict.v1 merge-approved sha256:3342ae3f566af11dd51da79d35e6bba3ebb8aa5da4d5119b2d1b1df36d5a2313 reviewer:human reviewer-id:probepark evidence:exact-head-a00ff6f4-exemption-post-budget-single-codepoint-allowlist-closes-nibble-argument-68-pass-0-fail

Lane retired. No release/tag/publish performed.


[repo owner's gaebal-gajae (clawdbot) 🦞]

Yeachan-Heo pushed a commit to lee98www/moss-gajae-code that referenced this pull request Aug 18, 2026
…ack continuation

Review of the reconstructed change found the steering installed only
after the fallbackManaged branch returns, so coding-agent sessions
(which run managed and own their retry through the session policy)
kept re-issuing the defective request blind. The deterministic Hangul
escaper cited in the PR body never saw the instruction.

The typed escaped_arguments_discarded outcome now reports whether the
discarded attempt still lacked a transient recovery instruction, and
the session's retry continuation attaches exactly one such instruction
through a new transientRecoveryMessage prompt option, consumed by the
loop as a one-shot synthetic recovery on the first assistant request of
the continuation. The instruction stays provider-only (never durable
history, never a later request) and the terminal per-call rejection
remains fail-closed; Yeachan-Heo#4627's decode-and-execute direction is still
rejected.

Lore-id: b7d2e4f1
Constraint: instruction must never enter durable history -- rides the existing synthetic recoveryMode path
Constraint: steering is one-shot per logical turn; never re-attach after a steered attempt was itself discarded
Rejected: steering every managed retry | a steered attempt that still escapes proves the instruction is not working; repeating it burns budget on identical requests
Rejected: decoding the escapes and executing | one mistyped nibble is unverifiable after parse
Confidence: high
Scope-risk: narrow
Reversibility: trivial
Tested: managed AgentSession regression (fails without the fix); all 25 agent escaped-guard tests; 790 agent package tests
Not-tested: none known
Yeachan-Heo pushed a commit to lee98www/moss-gajae-code that referenced this pull request Aug 20, 2026
…ack continuation

Review of the reconstructed change found the steering installed only
after the fallbackManaged branch returns, so coding-agent sessions
(which run managed and own their retry through the session policy)
kept re-issuing the defective request blind. The deterministic Hangul
escaper cited in the PR body never saw the instruction.

The typed escaped_arguments_discarded outcome now reports whether the
discarded attempt still lacked a transient recovery instruction, and
the session's retry continuation attaches exactly one such instruction
through a new transientRecoveryMessage prompt option, consumed by the
loop as a one-shot synthetic recovery on the first assistant request of
the continuation. The instruction stays provider-only (never durable
history, never a later request) and the terminal per-call rejection
remains fail-closed; Yeachan-Heo#4627's decode-and-execute direction is still
rejected.

Lore-id: b7d2e4f1
Constraint: instruction must never enter durable history -- rides the existing synthetic recoveryMode path
Constraint: steering is one-shot per logical turn; never re-attach after a steered attempt was itself discarded
Rejected: steering every managed retry | a steered attempt that still escapes proves the instruction is not working; repeating it burns budget on identical requests
Rejected: decoding the escapes and executing | one mistyped nibble is unverifiable after parse
Confidence: high
Scope-risk: narrow
Reversibility: trivial
Tested: managed AgentSession regression (fails without the fix); all 25 agent escaped-guard tests; 790 agent package tests
Not-tested: none known
pull Bot pushed a commit to nenyatech-mirror/gajae-code that referenced this pull request Aug 20, 2026
…nly tools (Yeachan-Heo#4627)

Restore the fail-closed terminal escapedNonAsciiArguments rejection for
every tool, then carve out exactly the motivating false positive: a tool
that declares displaySafeEscapedArgs (user-facing question text that
names no path, command, or identifier — currently only ask) executes
without resampling when its decoded arguments contain nothing but benign
typographic punctuation (curated set: U+2014 EM DASH). The json-parse
scanner stays strict and still flags every non-ASCII escape; mutating
tools and every other escaped character keep the two-resample budget and
terminal rejection. Composes with the retained-guard + steering
direction of Yeachan-Heo#4632, which owns transient recovery steering.

Red-team regressions cover: em-dash executes only on display-safe tools;
the same payload on a mutating tool never executes; the nibble-adjacent
en-dash (U+2013) and currency/math/full-width/separator/letter/emoji
escapes stay rejected even on display-safe tools.

Lore-id: 0f3c2a91
Constraint: mutating tools must never execute unverified \\uXXXX payloads
Constraint: scanner stays evidence-based; the exemption is decided on decoded args at execution time
Rejected: broad \p{P}\p{S}\p{Z} exemption | semantically significant nibble-sensitive symbols re-execute
Rejected: execute decoded args after budget globally | review 4957008249 major blocker
Confidence: high
Scope-risk: narrow
Reversibility: trivial
Tested: 28 escaped-nonascii agent tests, 787 agent suite, 125 targeted matrix, 100 ai targeted
Not-tested: none material
Closes: Yeachan-Heo#4627 review blockers

Co-authored-by: Yeachan-Heo <yeachan-heo@gajae.dev>
Yeachan-Heo pushed a commit to lee98www/moss-gajae-code that referenced this pull request Aug 20, 2026
…ack continuation

Review of the reconstructed change found the steering installed only
after the fallbackManaged branch returns, so coding-agent sessions
(which run managed and own their retry through the session policy)
kept re-issuing the defective request blind. The deterministic Hangul
escaper cited in the PR body never saw the instruction.

The typed escaped_arguments_discarded outcome now reports whether the
discarded attempt still lacked a transient recovery instruction, and
the session's retry continuation attaches exactly one such instruction
through a new transientRecoveryMessage prompt option, consumed by the
loop as a one-shot synthetic recovery on the first assistant request of
the continuation. The instruction stays provider-only (never durable
history, never a later request) and the terminal per-call rejection
remains fail-closed; Yeachan-Heo#4627's decode-and-execute direction is still
rejected.

Lore-id: b7d2e4f1
Constraint: instruction must never enter durable history -- rides the existing synthetic recoveryMode path
Constraint: steering is one-shot per logical turn; never re-attach after a steered attempt was itself discarded
Rejected: steering every managed retry | a steered attempt that still escapes proves the instruction is not working; repeating it burns budget on identical requests
Rejected: decoding the escapes and executing | one mistyped nibble is unverifiable after parse
Confidence: high
Scope-risk: narrow
Reversibility: trivial
Tested: managed AgentSession regression (fails without the fix); all 25 agent escaped-guard tests; 790 agent package tests
Not-tested: none known
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants